Use correct corpus versification for chapter calculation - #1021

This makes sense. Thank you, Peter! Will SF use this chapter filtering correctly though in cases where the source and target versifications differ? E.g., if the source corpus has a book with chapters 1, 2, 3, but the target splits 3 into 3 and 4, will the scripture range provided be BOOK1-3 on the source and BOOK1-4 on the target? A bit of me thinks that we should actually have the scripture ranges just follow the first source project since the source is the anchor between training and inferencing (i.e. you need source material for both). (If I'm understanding correctly, this bug would also keep you from drafting chapters that don't exist in the target, right?) Regardless, we should specify this in the documentation. I could see it either way. One would be simpler at the monolingual corpus level, but the other would be more straightforward at the build level. What do you think?

Related to sillsdev/machine#421.

@Enkidu93 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Will SF use this chapter filtering correctly though in cases where the source and target versifications differ? E.g., if the source corpus has a book with chapters 1, 2, 3, but the target splits 3 into 3 and 4, will the scripture range provided be BOOK1-3 on the source and BOOK1-4 on the target?

SF currently uses the source USFM for drafts, so will create books 1-3 in the target.

A bit of me thinks that we should actually have the scripture ranges just follow the first source project since the source is the anchor between training and inferencing (i.e. you need source material for both).

That was one option I tried (and did work - I am happy to implement that if you want). I chose to tie it to the corpus being fed into the mapper as it seemed more coherent. I figured if the target and source were training on chapters that only exist in the source, an error would be thrown and that's OK (and so I made it so the target corpus is used for versification calculation).

(If I'm understanding correctly, this bug would also keep you from drafting chapters that don't exist in the target, right?)

That is the bug this PR addresses?

Regardless, we should specify this in the documentation.

Yes.

I could see it either way. One would be simpler at the monolingual corpus level, but the other would be more straightforward at the build level. What do you think?

I saw this PR as a form of stop gap until sillsdev/machine#421 is completed. My impetus to create this PR was to bubble the error into the scope of that issue, as I think the problem could be solved far better than I have done here when we can transform versifications.

@pmachapman made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Rewriting as blockquotes messed up....

Will SF use this chapter filtering correctly though in cases where the source and target versifications differ? E.g., if the source corpus has a book with chapters 1, 2, 3, but the target splits 3 into 3 and 4, will the scripture range provided be BOOK1-3 on the source and BOOK1-4 on the target?

SF currently uses the source USFM for drafts, so will create books 1-3 in the target.

A bit of me thinks that we should actually have the scripture ranges just follow the first source project since the source is the anchor between training and inferencing (i.e. you need source material for both).

That was one option I tried (and did work - I am happy to implement that if you want). I chose to tie it to the corpus being fed into the mapper as it seemed more coherent. I figured if the target and source were training on chapters that only exist in the source, an error would be thrown and that's OK (and so I made it so the target corpus is used for versification calculation).

(If I'm understanding correctly, this bug would also keep you from drafting chapters that don't exist in the target, right?)

That is the bug this PR addresses?

Regardless, we should specify this in the documentation.

Yes.

I could see it either way. One would be simpler at the monolingual corpus level, but the other would be more straightforward at the build level. What do you think?

I saw this PR as a form of stop gap until Add capacity to transform USFM into a different versification sillsdev/machine#421 is completed. My impetus to create this PR was to bubble the error into the scope of that issue, as I think the problem could be solved far better than I have done here when we can transform versifications.

@pmachapman made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Sorry - I think I misunderstood. I thought that this error appeared when specifying a training filter, not an inferencing filter. Like you say, for inferencing, I think transforming the USFM into a different versification is the right solution to this problem. My question is more about how this change affects training filters:

If our source project is English and our target is Original and SF supplies a ScriptureRange of MAL1-3 on the source and MAL1-3 on the target, with this change, won't we only train on MAL1-3:18 and leave out the Original's MAL3:19-24? If we wanted to train on the whole book, we would now need to specify MAL1-4 for the source and MAL1-3 for the target, right? My point is that once the scripture ranges are parsed per the versification of that project (source or target), they may need to be different on the source and target for training. This, of course, could happen at the book level too. Is SF prepared to handle this properly? The only reason I suggested using the source versification as the versification for all scripture ranges is to avoid this problem.

@Enkidu93 made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).